Repository navigation
refactor: centralize lock release and simplify error publication - #121
Merged
Merged
Conversation
Move the enqueue phase of writeFrameInternal into a dedicated sendWriteRequest helper so writeReqLock is released by a single defer instead of four manual RUnlock calls. On a failed enqueue the request never reaches the shaper, so writeFrameInternal recycles resultCh via the recycle label and returns; the wait phase is unchanged and still refuses to recycle resultCh on die, write error or deadline because the request may already be in flight. Benefits: - Eliminates four manual RUnlock sites, each a potential lock leak if a future return path forgets to release. - Keeps resultCh recycling correct on both paths: returned to the pool only when the request definitely never reached the shaper, or after the result has been received. - Keeps the wait phase untouched, so the intentional "do not recycle" behaviour on die / write error / deadline is preserved. - Costs one extra non-inlined call (~1-2 ns) per frame. The defer blocks inlining and a goto rewrite of sendWriteRequest does not help either: the two select statements alone exceed the inline budget. The cost is negligible next to the channel send it wraps. Signed-off-by: Coia Prant <coiaprant@gmail.com>
Extract stream ID allocation from OpenStream into allocStreamID, and stream teardown from Close into closeStreams, so both lock/unlock pairs are covered by a single defer instead of being repeated on every return path. OpenStream now uses a cleanup label to funnel the three error paths (writeControlFrame failure, socket read error, socket write error, session die) through a single streamClosed call. The default branch still returns the wrapper without cleanup. No behavioural change: allocation order, overflow detection and the goAway flag are identical, and closeStreams keeps the same lock scope so Close still calls conn.Close and waits for chShaperDone outside the stream lock. Signed-off-by: Coia Prant <coiaprant@gmail.com>
The socket read, socket write and protocol errors are each written exactly once under sync.Once, before closing the corresponding notification channel, and are only read after a receive from that channel. The close/receive pair already establishes the happens-before edge required for safe publication, so atomic.Value is redundant here and only adds a type assertion on every load. Replace the three atomic.Value fields with plain error, and drop the type assertions at every read site. The sync.Once guards and the channel close semantics are unchanged, so the error is still stored exactly once and observed with the same visibility as before. Verified with go test -race. Signed-off-by: Coia Prant <coiaprant@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This series is pure refactoring, no behavioural change. It reduces the
number of manual lock release sites and drops an unnecessary atomic
type from the session error paths.
Patch 1 extracts the enqueue phase of writeFrameInternal into a
sendWriteRequest helper, so writeReqLock is released by a single defer
instead of four manual RUnlock calls on separate return paths. On a
failed enqueue the request never reaches the shaper, so the caller
recycles resultCh through the recycle label; the wait phase keeps its
intentional "do not recycle" behaviour on die, write error and
deadline, because the request may already be in flight. The defer
prevents sendWriteRequest from being inlined, and a goto rewrite does
not help either since the two select statements alone exceed the inline
budget. The extra non-inlined call costs about 1-2 ns per frame, which
is negligible next to the channel send it wraps.
Patch 2 extracts allocStreamID from OpenStream and closeStreams from
Close. allocStreamID keeps the goAway flag and the ID overflow check
under nextStreamIDLock, with a single defer replacing three manual
Unlock sites. OpenStream funnels its four error paths through a cleanup
label that calls streamClosed once, and returns the wrapper unchanged
on the success path. closeStreams keeps the same streamLock scope as
before, so Close still calls conn.Close and waits for chShaperDone
outside the stream lock.
Patch 3 replaces the three atomic.Value error fields (socket read,
socket write, protocol) with plain error. Each field is written exactly
once under sync.Once, before closing the corresponding notification
channel, and is only read after a receive from that channel. The
close/receive pair already provides the happens-before edge needed for
safe publication, so atomic.Value was redundant and only added a type
assertion on every load.
Verified with go test -race -short.